Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add consistency between cell plots tab and marker genes tab #388

Merged
merged 10 commits into from
May 9, 2024
Merged

Add consistency between cell plots tab and marker genes tab #388

merged 10 commits into from
May 9, 2024

Conversation

lingyun1010
Copy link
Contributor

@lingyun1010 lingyun1010 commented Mar 21, 2024 •
edited
Loading

According to Silvie's detailed comments, I add logics to make cell plot & marker gene plot options are consistent when the plot type is clusters.

Besides, I refactored the tab name from tsne to cell-plots as discussed in some meetings before, so the tab refers to both tsne and umap plots more properly.

@lingyun1010
Add logics to make cell plot & marker gene plot ..
5ffbf2f 
options are consistent when it is cluters plot type
@lingyun1010 lingyun1010 requested a review from ke4 March 21, 2024 16:08
@lingyun1010 lingyun1010 self-assigned this Mar 21, 2024
ke4
ke4 requested changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a tiny typo that I commented.
My bigger concern is that this JS file is not very readable.
I would like you to try to refactor this file using the knowledge you learnt either on the Uncle Bob or Venkat Subramaniam videos.
Many thanks,
Karoly

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js Outdated Show resolved Hide resolved
app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js Show resolved Hide resolved
@lingyun1010 lingyun1010 linked an issue Apr 2, 2024 that may be closed by this pull request
Inconsistency between cell plot and heat map #381
Closed
@ke4
Copy link
Contributor

ke4 commented Apr 15, 2024

Your latest modification improved the code a bit, but we could do much more with it.
I was thinking on this a bit, and I TBH it is going to be easier if I am going to sit down with you and show you how to clean this code. We can do it tomorrow if you are going to be in the Campus. It will only take a couple of minutes to show you and after that probably 10-15 minutes of your time to make the code prettier.
Thanks

ke4
ke4 requested changes Apr 24, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a suggestion that you can quickly apply with the use of the button.
I think there are other things we could improve, but I don't want to spend more time on this one, so I am going to approve it after you applied my suggestion.
Thanks

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js Outdated Show resolved Hide resolved
@lingyun1010 lingyun1010 requested a review from ke4 May 1, 2024 12:29
ke4
ke4 approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lingyun1010 lingyun1010 merged commit b03ab96 into develop May 9, 2024
2 checks passed
@lingyun1010 lingyun1010 deleted the 381-inconsistency-between-cell-plot-and-heat-map branch May 9, 2024 14:47
@ke4 ke4 mentioned this pull request May 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ke4 ke4 ke4 approved these changes

Assignees

@lingyun1010 lingyun1010

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistency between cell plot and heat map
2 participants
@lingyun1010 @ke4